net/streams: don't return the stream object from onStreamRead #34375
Conversation
|
Is it possible to add a regression test? |
|
i think you would need to have some integration tests, sadly. it doesn't seem possible for a connection to be "reset" between two sockets on the same machine. (my understanding is that ECONNRESET is a response to unexpectedly receiving a TCP RST.) another way you could do it is to fake up the ECONNRESET by mucking around in the internals somehow, but i'm not sure how you can trick libuv into doing that. would be cool tho! |
|
I think |
There was a problem hiding this comment.
There is another instance of this here https://github.com/nodejs/node/pull/34375/files#diff-704db381d3305bdcd2c6b8b3468acfb7R230
|
good eye! i went ahead and fixed that one too, while i was in here. |
robey
changed the title
|
Can you please add a unit test? |
|
as i said above, if you can think of a way to do it without having a cross-process integration test, i'm happy to try it. |
|
would it be possible to use child processes? We have a few tests that use them for similar purposes. |
|
i figured out some sample code that causes the crash (see bug), so i added that as a unit test. :) |
sure, added that too. :) |
addaleax
added
the
author ready
|
Looks like there are a number of possibly relevant failures in CI |
jasnell
added
net
i assumed these were all spurious since "make test" passes cleanly (except the one that always fails). which test is failing for you and how can i repro it? |
|
@robey here are a few examples: node-test-commit-freebsd node-test-commit-smartos node-test-commit-windows-fanned |
src/stream_base.cc
Outdated
| @@ -538,7 +538,8 @@ void CustomBufferJSListener::OnStreamRead(ssize_t nread, const uv_buf_t& buf) { | |||
| 0, | |||
| StreamBase::SKIP_NREAD_CHECKS); | |||
| Local<Value> next_buf_v; | |||
| if (ret.ToLocal(&next_buf_v) && !next_buf_v->IsUndefined()) { | |||
| if (ret.ToLocal(&next_buf_v) && !next_buf_v->IsUndefined() && | |||
| next_buf_v->IsArrayBufferView()) { | |||
There was a problem hiding this comment.
Might this be what's causing problems?
There was a problem hiding this comment.
makes sense... ok, i reverted that part.
CallJSOnreadMethod expects the return value to be undefined or a new buffer, so make sure to return nothing, even when an error causes us to destroy the stream. Fixes: #34346
|
it doesn't seem like there's any interest in this patch, so i'll close it next week. we can run a fork of node internally. sorry it didn't work out. |
|
CI is green. I think this can land. |
lpinca
added
the
author ready
|
Landed in c0be31f, and sorry for the delay! |
CallJSOnreadMethod expects the return value to be undefined or a new buffer, so make sure to return nothing, even when an error causes us to destroy the stream. Fixes: #34346 PR-URL: #34375 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
CallJSOnreadMethod expects the return value to be undefined or a new buffer, so make sure to return nothing, even when an error causes us to destroy the stream. Fixes: #34346 PR-URL: #34375 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
CallJSOnreadMethod expects the return value to be undefined or a new buffer, so make sure to return nothing, even when an error causes us to destroy the stream. Fixes: #34346 PR-URL: #34375 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
CallJSOnreadMethod expects the return value to be undefined or a new buffer, so make sure to return nothing, even when an error causes us to destroy the stream. Fixes: #34346 PR-URL: #34375 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
CallJSOnreadMethod expects the return value to be undefined or a new buffer, so make sure to return nothing, even when an error causes us to destroy the stream. Fixes: #34346 PR-URL: #34375 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
CallJSOnreadMethod expects the return value to be undefined or
a new buffer, so make sure to return nothing, even when an error
causes us to destroy the stream.
Fixes: #34346
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes